Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Executor: Remove duplicate entry with dry-run argument #4660

Merged
merged 7 commits into from
Aug 24, 2022

Conversation

mmacchia
Copy link
Contributor

@mmacchia mmacchia commented Oct 20, 2021

Pull Request Check List

Resolves: #3097

  • Added tests for changed code. I adapted the test in the tests/console/commands/plugin/test_remove.py file to reflect the bug fix.
  • Updated documentation for changed code.

Apart from running the ci linting steps and pytest locally for both python 3.6 and 3.8, I also tested the code by running

docker run --rm -i --entrypoint bash python:3.8 <<EOF
set -e
python -m pip install -q git+https://github.com/mmacchia/poetry.git@issue/3097
python -m poetry new foobar
pushd foobar
sed -i /pytest/d pyproject.toml
python -m poetry add --dry-run pycowsay
python -m poetry run pip list
EOF

The corresponding output is:

Created package foobar in foobar
/foobar /
Creating virtualenv foobar-lWDpn5M1-py3.8 in /root/.cache/pypoetry/virtualenvs
Using version ^0.0.0.1 for pycowsay

Updating dependencies
Resolving dependencies...

Writing lock file

Package operations: 1 install, 0 updates, 0 removals

  • Installing pycowsay (0.0.0.1)
Package    Version
---------- -------
pip        21.2.4
setuptools 58.1.0
wheel      0.37.0

As expected, the command poetry add --dry-run pycowsay yielded a single output ( • Installing pycowsay (0.0.0.1)) and did not add the package to the project (see the output of poetry run pip list).

The change in the PR solves the duplication issue because the creation of the console output when not self._enabled is taken care of by the boolean output of theself._should_write_operation(operation) function.

Please feel free to get in touch if I can help adding more test cases for the --dry-run argument or the _enabled flag and to add any feedback. I would be happy to improve/clarify the code in this PR.

@mmacchia
Copy link
Contributor Author

mmacchia commented Oct 20, 2021

Moreover, I tested the difference of the new and old code on the pyproject.toml file presented in the original issue, link.

Please compare the output of

docker run --rm -i --entrypoint bash python:3.8 <<EOF
set -e
python -m pip install -q git+https://github.com/mmacchia/poetry.git@issue/3097
python -m poetry new foobar
pushd foobar
curl https://gist.githubusercontent.com/MarcoGlauser/804cbf48001f7a7d1d94d6da9bc88071/raw/87d3273a0e7b86d03023b2dfcc85567b80161d3d/pyproject.toml > pyproject.toml
python -m poetry update --dry-run
python -m poetry run pip list
EOF

and

docker run --rm -i --entrypoint bash python:3.8 <<EOF
set -e
python -m pip install -q git+https://github.com/python-poetry/poetry.git@master
python -m poetry new foobar
pushd foobar
curl https://gist.githubusercontent.com/MarcoGlauser/804cbf48001f7a7d1d94d6da9bc88071/raw/87d3273a0e7b86d03023b2dfcc85567b80161d3d/pyproject.toml > pyproject.toml
python -m poetry update --dry-run
python -m poetry run pip list
EOF

@mmacchia
Copy link
Contributor Author

mmacchia commented Oct 22, 2021

One last question: as I said above, I successfully run the commands poetry run pytest tests and poetry run pre-commit run --all-files both in a python v3.6 and v3.8 environment. However it seems that the tox CI pipeline failed on v3.6 with a ('Connection aborted.', ConnectionResetError(54, 'Connection reset by peer')) error. Is there any way to address this? Is it possible to re-trigger the build on python 3.6? It looks like an SSL issue in the instance provisioned for CI.

@mmacchia
Copy link
Contributor Author

I finally rebased my branch and the tests now passed in the CI instance, thank you for updating them! Please feel free to have a look at the suggested changes now.

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mmacchia -- could you please rebase this so it can be merged into master?

@mmacchia mmacchia marked this pull request as ready for review June 4, 2022 10:57
@mmacchia
Copy link
Contributor Author

mmacchia commented Jun 4, 2022

Hi @mmacchia -- could you please rebase this so it can be merged into master?

Hello @neersighted ! Thank you for picking this up. I managed to rebase after some troubleshooting.
Please feel free to review the changes now.

Would it be possible to squash the commits when you believe this PR is ready to be merged?

Copy link
Member

@branchvincent branchvincent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate entries when running poetry update --dry-run
4 participants